fix(express): Ensure 404 routes don't attach route attribute #2843
Conversation
pichlermarc
added
bug
There was a problem hiding this comment.
LGTM! thank you for your contribution @s1gr1d 🙂
cc @raphael-theriault-swi, @JamieDanielson, @pkanal (component owners)
|
oh - looks like there are some lint errors, you can fix them by running |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2843 +/- ##
==========================================
- Coverage 97.62% 89.72% -7.91%
==========================================
Files 9 187 +178
Lines 505 9097 +8592
Branches 89 1879 +1790
==========================================
+ Hits 493 8162 +7669
- Misses 12 935 +923
🚀 New features to boost your workflow:
|
|
I have to look into this again. I see another test is failing. It doesn't add |
|
The problem was that the layers look the same if a non-existing route is requested or the root is requested: So in order to know if it was a 404 route, I added a utility function that does some additional checking. I made sure that the existing tests still work and added some more tests for the utility function. |
There was a problem hiding this comment.
Hi @s1gr1d, thanks for following up on this.
It seems that this PR is based on a commit that's a little over 2 months old. Would you mind rebasing it on top of main it so that we can see how this would fit in with the current state? 🙂
s1gr1d
force-pushed
the
sigrid/handle-express-404
branch
from
6592b20 to
99a94d2
Compare
s1gr1d
force-pushed
the
sigrid/handle-express-404
branch
from
99a94d2 to
006539c
Compare
| if (constructedRoute.includes('/') && | ||
| (constructedRoute.includes(',') || | ||
| constructedRoute.includes('\\') || | ||
| constructedRoute.includes('*') || | ||
| constructedRoute.includes('['))) { |
There was a problem hiding this comment.
This condition is for considering regex routes (without this, the regex tests would fail). This should cover most cases.
There is a previous conversation about this: #2008 (comment)
|
@pichlermarc I rebased it and aligned the code so the "regex" tests don't fail anymore. |
| const layersStore: string[] = Array.isArray(req[_LAYERS_STORE_PROPERTY]) | ||
| ? (req[_LAYERS_STORE_PROPERTY] as string[]) | ||
| : []; |
There was a problem hiding this comment.
nit: _LAYERS_STORE_PROPERTY is set only in storeLayerPath function. Meaning that if defined it must be an array. So this const assignment could be simplified to.
const layersStore = req[_LAYERS_STORE_PROPERTY] || [];I see you added a test specifically for a scenario where that property is defined with a non-array value. Probably to avoid a case where another piece of code outside the instrumentation modifies the property. If you want the extra safety of Array.isArray it's okay but there is no need to cast types. TS resolves the type properly from the function signature.
const layersStore = Array.isArray(req[_LAYERS_STORE_PROPERTY])
? req[_LAYERS_STORE_PROPERTY]
: [];NOTE: maybe if the instrumentation uses a Symbol instead of a string it will ensure no other code could change the layer store
| originalUrl: PatchedRequest['originalUrl']; | ||
| [_LAYERS_STORE_PROPERTY]?: string[]; | ||
| }): string | undefined { | ||
| const layersStore: string[] = Array.isArray(req[_LAYERS_STORE_PROPERTY]) |
6 participants
Which problem is this PR solving?
This is an upstream fix for getsentry/sentry-javascript#16211
Instead of adding
/to non-existing routes, it sets the attribute toundefined, which makes it clear it's a non-existing route and not the index route.A tricky thing is that the layers look the same if a non-existing route is requested or the root is requested:
['/', '/']./existing-->['/', '/existing']/non-existing-->['/', '/']/-->['/', '/']So in order to know if it was a 404 route, a utility function that does some additional checking was added.